Column visibility group #241
Open
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This merge request introduces the Column Visibility Groups feature, which allows users to organize table columns into different "views" and switch between them using a dropdown in the UI as proposed in #86. This is particularly useful for tables with many columns, enabling a cleaner and more user-friendly display.
Check the documentation to know how this works
Implementation Notes & Points of Attention
I have done my best to follow the bundle's structure, but I would appreciate feedback on a few specific points:
src/DataTable.php:
private ?string $requestedColumnVisibilityGroup = null;Is this the right place for this property? It is used to keep track of the column visibility group selected by the user.
src/DataTableBuilder.php (line 969):
I use
ColumnVisibilityGroupBuilderto directly createColumnVisibilityGroupinstances. I did not use a factory, as it did not seem necessary here. Is this approach acceptable?src/Type/DataTableType.php (line 121):
I pass the
ColumnVisibilityGroupsdirectly to the view, without creating aColumnVisibilityGroupView. Do you think a dedicated view class is needed, or is this sufficient?src/ColumnVisibilityGroup/ColumnVisibilityGroupBuilder.php (line 26):
I use the translator, but currently do not allow the use of the
translation_domaindefined in theDataTableType, nor do I allow overriding thetranslation_domainor adding arguments. Should this be improved to support these features?Testing:
I have not yet created tests for this feature. I would appreciate advice on which aspects should be covered by tests and any recommendations for structuring them.
Documentation:
The documentation could potentially be expanded further. Please let me know if there are areas that require more detail or clarification.
Request for Feedback
Thank you for your review and feedback!